UnitControl: fix controlled unit behavior
#39148
Conversation
ciampo
added
[Type] Bug
ciampo
force-pushed
the
fix/unit-control-unit-controlled-uncontrolled
branch
from
16b609f to
39e95dd
Compare
|
Size Change: +17 B (0%) Total Size: 1.15 MB
ℹ️ View Unchanged
|
|
Can we fix Changes LGTM 🚀 |
There was a problem hiding this comment.
Nice fix @ciampo, thanks for the thoroughness with the addition of the storybook example and test coverage 👍 — that's all testing well for me, as is standard usage of the component in the block editor.
Aside from the linting issue in package.json this looks good to me, so I'll give it the ✅.
I like the addition of user-event, too, it's quite a bit easier to work with than fireEvent for more complex interactions. I'll just CC @gziolo, though, as he had feedback on another PR I believe that also was looking at adding in another devDependency (from memory, I think it was confidence checking how common it'll be to use the new things we add in as dependencies). In this case, since it looks like React Testing Library recommends using user-event most of the time in favour of fireEvent (here in the docs), it sounds like it'll be worth it to add it in?
ciampo
force-pushed
the
fix/unit-control-unit-controlled-uncontrolled
branch
from
dddf4be to
19a870a
Compare
|
Thank you for the quick review! I changed the order of the dependencies (somehow it wasn't flagged locally on my machine), and rebased to fix a conflict.
That's what I thought as well — |
There is one thing I don't like about the recommendations they included:
It too often leads to fragmentation in projects where you have old ways of writing tests, and suddenly there is a better way that impacts the development process because some people might consider it a best practice that everyone should follow. We still have tests written in Enzyme that were never refactored despite the fact that the project is no longer maintained by original creators. It looks like there is a growing need for having better testing tools so feel free to land this library as I see there is an agreement that the benefits are bigger than the potential maintenance cost. As far as I can tell, this library depends only on For the future, I encourage you to make this type of proposal more transparent. A separate targeted PR with a refactored example test(s), documentation, a list of pros and cons included would give other contributors a chance to leave their feedback. |
Hey @gziolo , you definitely make a good point here. I am also happy to remove the dependency, rewrite the tests with |
Both are fine. I don't have strong opinions on that at this point since I'm not opposed to the idea after a short investigation I did in regards to this small utilities library 😄 |
|
Update: I removed I'll open a separate PR in the upcoming days about the introduction of the library to Gutenberg, highlighting the pros and showcasing a couple of examples. I'll merge once CI is 🟢 |
| @@ -25,6 +27,47 @@ const getUnitLabel = () => | |||
| const fireKeyDown = ( data ) => | |||
| fireEvent.keyDown( document.activeElement || document.body, data ); | |||
|
|
|||
| const ControlledSyncUnits = () => { | |||
There was a problem hiding this comment.
Non-blocking: Was this story only useful for testing this particular fix or do you think there is value in keeping it around? It's not immediately clear to me whether the main purpose of the story is to demonstrate a particular behavior/implementation, or to provide a debugging playground moving forward for the synced usage. My personal impression is that unit tests are sufficient and better for this kind of behavioral QA, unless you expect frequent manual testing of this "controlled" case in the near future.
There was a problem hiding this comment.
You make a good point. This story is actually very targeted to test the buggy behaviour, and probably doesn't bring much value to Storybook users. I'll go ahead and remove it from this PR.
|
Thanks for the fix @ciampo! |
Description
In #37311 (comment), @ntsekouras flagged a bug where the
UnitControlcomponent wouldn't correctly update its unit if the update was coming via thevalueprop (as opposed as from a user interaction).For reference, these two patterns are also referred to as "controlled" or "uncontrolled" components.
UnitControlshould technically already support this behaviour, but the bug report shows otherwise.This PR introduces a fix around that, alongside a new Storybook example and a unit test that mimic closely the scenario encountered by Nik when flagging the buggy behavior.
This PR also introduces a new dev dependency,
@testing-library/user-event, a recent addition to Testing Library for handling interaction in unit tests better.Potential follow-ups:
NumberControlto support min/max font size forTag Cloud Block#37311, rewriting the code in a way that doesn't use the deprecatedunitpropunitprop as deprecated also in TypeScript types, and consider adding a soft deprecation warning toouser-eventlibrary (and convert to TypeScript)Testing Instructions
UnitControlunit tests, and make sure they passUnitControland make sure it works as expected (changing the unit in one control triggers that same unit change in the other control)Comment out the fix added by this PR to the component:
Screenshots
Before:
unit-control-controlled-before.mp4
After:
unit-control-controlled-after.mp4
Types of changes
Checklist:
*.native.jsfiles for terms that need renaming or removal).